Histogram breaks #767

bbeat2782 · 2023-07-28T17:04:31Z

Describe your changes

Implemented --breaks/-B to ggplot histogram.

breaks demonstration.zip This notebook contains breaks demonstration and errors I implemented.

Issue number

Closes #719

Checklist before requesting a review

Performed a self-review of my code
Formatted my code with pkgmt format
Added tests (when necessary).
Added docstring documentation and update the changelog (when needed)

📚 Documentation preview 📚: https://jupysql--767.org.readthedocs.build/en/767/

bbeat2782 · 2023-07-28T17:27:34Z

breaks demonstration.zip

This is the implemented behavior of breaks. @edublancas Could you take a look at the demonstration and check whether it behaves as we want when you have time?

Since I have two other issues, this does not need an immediate check.

edublancas

looks great! don't forget to add some reference images and test cases

doc/api/magic-plot.md

edublancas · 2023-07-29T01:14:04Z

src/sql/magic_plot.py

+        bin_specified = False
+        pattern = r"histogram.*( --bins | -b )"
+        if re.search(pattern, line):
+            bin_specified = True
+            if re.search(r"( --breaks | -B )", line):
+                raise exceptions.UsageError(
+                    "Both bins and breaks are specified. Must specify only one of them."
+                )
+


I think you can use cmd to check if both arguments exist.

e.g.,

if cmd.args.bins and cmd.args.breaks: raise UsageError(...)

let me know if that works, otherwise we should fix it since validating arguments with regular expressions is going to be a pain if we have more complex combinations

Because of the default value of bins (which is 50 for the magic command), cmd.args.bins always return an integer value. Thus, I used the following code instead.

bin_specified = " --bins " in line or " -b " in line breaks_specified = " --breaks " in line or " -B " in line if bin_specified and breaks_specified: raise exceptions.UsageError(...)

changelog test cases and fixes comments added

tonykploomber

LG, very comprehensive test case covered, GJ

src/sql/plot.py

neelasha23 · 2023-08-02T07:13:44Z

Would it make sense to allow users to pass a range of values?

points = list(range(1000, 6000, 500))
%sqlplot histogram --table penguins.csv --column body_mass_g --breaks {{points}}

@edublancas @bbeat2782

A few more observations:

I'm not able to understand this result: If we specify %sqlplot histogram --table penguins.csv --column body_mass_g --bins 100 the plot is as below (x value goes upto 6000+)

But when we specify breaks where last break is less than highest x value it doesn't show data beyond the last point:

Shouldn't there be one more bin after 5000 till the highest value of body_mass_g ?

UsageError: All break points are lower than the min data point. : It might help the user if we specify what the minimum point is (similar for max data point).
There would be only 1 bin if first point is less than lower datapoint and second point is higher than max value. We can provide some info to guide user when specifying points:

src/sql/plot.py

yafimvo

Looks good.

Added some comments.

I think we should add some integration tests and make sure it works (or doesn't) on other databases (PostgreSQL, questdb, etc...)

src/sql/ggplot/geom/geom_histogram.py

src/sql/magic_plot.py

src/sql/plot.py

yafimvo · 2023-08-02T08:45:18Z

src/sql/plot.py

+        if bins:
+            raise ValueError(
+                "Both bins and breaks are specified. Must specify only one of them."
+            )


This error repeats itself. Can we delete the error handler from here?

Also, please change ValueError to exceptions.ValueError

Deleted the error handler in magic_plot and changed ValueError to exceptions.ValueError

doc/api/magic-plot.md

bbeat2782 · 2023-08-02T19:32:33Z

@neelasha23

allow users to pass a range of values

With this PR implementation, the following works.

points = " ".join(map(str, range(1000, 6000, 500)))
%sqlplot histogram --table penguins.csv --column body_mass_g --breaks {points}

Shouldn't there be one more bin after 5000 till the highest value of body_mass_g ?

When specifying breaks, we are specifying a starting point, break points, and ending point. Thus, the data we query is limited to starting point <= data <= ending point. That is why the upper limit is 5000 here. This range limit is mimicking ggplot behavior in R. To avoid confusion, I added an explanation of breaks limiting range in magic-plot.md.

It might help the user if we specify what the minimum point is

Changed the error message to UsageError: All break points are lower than the min data point of {min point}. (similar to the max data point)

There would be only 1 bin if first point is less than lower datapoint and second point is higher than max value. We can provide some info to guide user when specifying points:

For this, do you think displaying a message will work better or giving a general info on the docs will work better?

bbeat2782 · 2023-08-02T19:42:42Z

@yafimvo

I think we should add some integration tests and make sure it works (or doesn't) on other databases (PostgreSQL, questdb, etc...)

Added test cases and images in test_questDB.py.

edublancas · 2023-08-02T22:29:02Z

@neelasha23 @yafimvo please review again

neelasha23 · 2023-08-03T07:31:57Z

For this, do you think displaying a message will work better or giving a general info on the docs will work better?

Docs should be fine

neelasha23

Looks good!

edublancas

looks good, one thing is missing is adding one new test here:

jupysql/src/tests/integration/test_generic_db_operations.py

Line 295 in b6f07dc

"histogram-bins",

you can just add an example that runs using --breaks, let's see if it passes. if you find compatibilities, ping me and we can decide whether we support it or not. my guess is that it'll work with the databases that we're testing there

bbeat2782 · 2023-08-04T17:48:39Z

one thing is missing is adding one new test here:

@edublancas I added histogram-breaks, and it passed all tests

edublancas · 2023-08-05T00:21:09Z

awesome!

edublancas requested changes Jul 29, 2023

View reviewed changes

ggplot histogram breaks

48cb68d

changelog test cases and fixes comments added

bbeat2782 marked this pull request as ready for review July 31, 2023 20:57

bbeat2782 requested review from neelasha23 and yafimvo as code owners July 31, 2023 20:57

bbeat2782 requested a review from edublancas July 31, 2023 20:57

Merge branch 'master' into histogram_breaks

3234079

tonykploomber previously approved these changes Aug 1, 2023

View reviewed changes

neelasha23 reviewed Aug 2, 2023

View reviewed changes

src/sql/plot.py Show resolved Hide resolved

neelasha23 reviewed Aug 2, 2023

View reviewed changes

src/sql/plot.py Show resolved Hide resolved

yafimvo requested changes Aug 2, 2023

View reviewed changes

fix

88e0440

bbeat2782 dismissed tonykploomber’s stale review via 88e0440 August 2, 2023 17:31

bbeat2782 added 2 commits August 2, 2023 10:35

Merge remote-tracking branch 'upstream/master' into histogram_breaks

a619fc5

fix

6a4e255

bbeat2782 requested review from neelasha23 and yafimvo August 2, 2023 19:13

neelasha23 previously approved these changes Aug 3, 2023

View reviewed changes

edublancas requested changes Aug 4, 2023

View reviewed changes

bbeat2782 added 2 commits August 4, 2023 10:23

Merge remote-tracking branch 'upstream/master' into histogram_breaks

cc761f9

adding one test in generic_db_operations

9986b69

bbeat2782 dismissed neelasha23’s stale review via 9986b69 August 4, 2023 17:33

bbeat2782 requested a review from edublancas August 4, 2023 17:48

edublancas approved these changes Aug 5, 2023

View reviewed changes

edublancas merged commit 424ff48 into ploomber:master Aug 5, 2023
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Histogram breaks #767

Histogram breaks #767

bbeat2782 commented Jul 28, 2023 •

edited

Loading

bbeat2782 commented Jul 28, 2023

edublancas left a comment

edublancas Jul 29, 2023

bbeat2782 Jul 31, 2023

tonykploomber left a comment

neelasha23 commented Aug 2, 2023 •

edited

Loading

yafimvo left a comment

yafimvo Aug 2, 2023

bbeat2782 Aug 2, 2023

bbeat2782 commented Aug 2, 2023

bbeat2782 commented Aug 2, 2023

edublancas commented Aug 2, 2023

neelasha23 commented Aug 3, 2023

neelasha23 left a comment

edublancas left a comment

bbeat2782 commented Aug 4, 2023

edublancas commented Aug 5, 2023

Histogram breaks #767

Histogram breaks #767

Conversation

bbeat2782 commented Jul 28, 2023 • edited Loading

Describe your changes

Issue number

Checklist before requesting a review

bbeat2782 commented Jul 28, 2023

edublancas left a comment

Choose a reason for hiding this comment

edublancas Jul 29, 2023

Choose a reason for hiding this comment

bbeat2782 Jul 31, 2023

Choose a reason for hiding this comment

tonykploomber left a comment

Choose a reason for hiding this comment

neelasha23 commented Aug 2, 2023 • edited Loading

yafimvo left a comment

Choose a reason for hiding this comment

yafimvo Aug 2, 2023

Choose a reason for hiding this comment

bbeat2782 Aug 2, 2023

Choose a reason for hiding this comment

bbeat2782 commented Aug 2, 2023

bbeat2782 commented Aug 2, 2023

edublancas commented Aug 2, 2023

neelasha23 commented Aug 3, 2023

neelasha23 left a comment

Choose a reason for hiding this comment

edublancas left a comment

Choose a reason for hiding this comment

bbeat2782 commented Aug 4, 2023

edublancas commented Aug 5, 2023

bbeat2782 commented Jul 28, 2023 •

edited

Loading

neelasha23 commented Aug 2, 2023 •

edited

Loading